fix(core): reject module nodes with source fields - #2911

Merging to main in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Automated review by Claude. No issues found.

validate_module_node_source_fields rejects a module node that also carries path / operators / operator / custom / ros2, which are mutually exclusive with module. All five fields exist on the Node struct, and running the check as the first statement of expand_module_node also covers nested module nodes reached through recursion. The two-scenario test checks both a path and an operator conflict and asserts on the offending field name. Correct and useful.

🤖 This is a fully automated review by Claude (Claude Code). No human has vetted these findings — please verify before acting on them.

Generated by Claude Code

Generated by Claude Code

The conflict check is incomplete: it rejects path / operators / operator / custom / ros2 but misses other source/build fields on the same Node struct that are equally mutually exclusive with module.

• libraries/core/src/descriptor/expand.rs:635 (validate_module_node_source_fields): does not check node.hub, node.git, node.build, or node.path_sha256.
  • hub (descriptor.rs:681) is a direct source alternative to path — a node like module: m.yml + hub: some/ref is silently accepted, and hub is dropped.
  • git (descriptor.rs:655) / build (descriptor.rs:624) / path_sha256 (descriptor.rs:311) are source/build specifiers that are likewise not consumed for a module node during expansion (only deploy is propagated from the module node, and module-level build comes from module_file.build, not node.build). They are silently ignored rather than rejected.
• Net effect: the error message claims to catch "fields that are mutually exclusive with module", but a user who attaches hub/git/build to a module node gets no error and silent field loss — the exact confusion this PR aims to prevent. Consider adding these fields to the conflicts list (or asserting on the full allow-list of legal module-node fields).

The two-scenario test is otherwise meaningful (covers path and operator, asserts on the offending field name).

Generated by Claude Code

Automated review by Claude — fully automated; not vetted by a human.

The latest revision addresses my earlier point: validate_module_node_source_fields now also rejects hub, git, build, and path_sha256 (plus branch/tag/rev), so the check covers the full set of source/build fields on Node that are mutually exclusive with module. The build-propagation refactor (skipping the module-level build prepend for nested module nodes and propagating module_file.build directly) is behavior-equivalent and consistent with module nodes now rejecting a build field. No further issues found.

Generated by Claude Code

Automated review by Claude — this is a fully automated review with no human in the loop; please verify before acting. (Reviewing the field-validation code added in the latest commit, which post-dates the previous review.)

I found the following issue:

validate_node_fields_for_kind turns a previously-graceful error into a panic for un-expanded module nodes. In libraries/core/src/descriptor/mod.rs, resolve_aliases_and_set_defaults iterates every node with no module filtering (for mut node in self.nodes.clone()), and now calls validate_node_fields_for_kind(&node)? before node_kind_mut(&mut node)?.

• validate_node_fields_for_kind opens with let kind = node.kind()?;, and Node::kind() returns Ok(NodeKind::Module(_)) for a node with module: set (mod.rs:588).
• Its NodeKind::Module(_) arm is unreachable!("module node must be expanded before resolution").
• But this state is reachable: node_kind_mut — the call that used to run first — deliberately handles it with a graceful eyre::bail!("module node \{}` must be expanded before resolution — call expand_modules() first")` (mod.rs:326-331).

resolve_aliases_and_set_defaults is a public DescriptorExt method, so any caller that resolves a descriptor still containing a module node (i.e. before expand_modules()) now hits unreachable! and panics instead of getting the Err it got before. Suggest replacing the unreachable! with the same eyre::bail! that node_kind_mut uses, preserving the error-return contract.

Minor (not correctness): with validate_module_node_fields added to expand_module_node, the immediately-following validate_module_node_source_fields(node)? call is now redundant — every field it checks (path/path_sha256/build/git/hub/branch/tag/rev/operators/operator/custom/ros2) is already rejected by validate_module_node_fields, which runs first.

Generated by Claude Code

🤖 Automated review by Claude — this is a fully automated review, with no human in the loop.

The latest revision only adds a formatting-only commit (07f2014), so the panic regression flagged in the previous review is still present and unfixed:

validate_node_fields_for_kind (in libraries/core/src/descriptor/mod.rs) opens with let kind = node.kind()?;. For a module node, Node::kind() returns Ok(NodeKind::Module(_)) (see the (None, None, None, None, None, Some(module)) => Ok(NodeKind::Module(module)) arm), and the corresponding arm in validate_node_fields_for_kind is:

NodeKind::Module(_) => {
    unreachable!("module node must be expanded before resolution");
}

resolve_aliases_and_set_defaults iterates every node with no module filtering (for mut node in self.nodes.clone()) and now calls validate_node_fields_for_kind(&node)? before node_kind_mut(&mut node)? — the call that previously handled this exact state with a graceful eyre::bail!("module node ... must be expanded before resolution — call expand_modules() first").

Since resolve_aliases_and_set_defaults is a public DescriptorExt method, any caller that resolves a descriptor still containing a module node (i.e. one that didn't call expand_modules() first) now panics instead of getting an Err, changing the method's error-return contract into an abort. Replacing the unreachable! with the same eyre::bail! used by node_kind_mut would preserve the previous behavior.

Minor (also noted previously): in expand_module_node, validate_module_node_source_fields(node)? is now dead code — every field it checks (path, path_sha256, build, git, hub, branch, tag, rev, operators, operator, custom, ros2) is already rejected by validate_module_node_fields, which runs a few lines earlier and bails first. The second function can never fire.

Generated by Claude Code

Follow-up review of the new commit 961360e (there were no code changes to re-review before it; it's the only push since the last automated review).

It resolves both issues raised earlier:

• The NodeKind::Module(_) arm in validate_node_fields_for_kind now returns eyre::bail!(...) instead of unreachable!, so passing an un-expanded module node to the public resolve_aliases_and_set_defaults returns an Err again rather than aborting the process.
• validate_module_node_source_fields has been removed — its checks were a strict subset of validate_module_node_fields (which runs immediately before it), so it was dead code and the removal is behavior-preserving.

I re-read the resulting diff and found no new issues. The latest commit looks safe to merge with respect to those two points.

Disclaimer: this is a fully automated review by Claude — no human has verified these findings. Please double-check before relying on them.

Generated by Claude Code

🤖 Fully automated review by Claude — no human has vetted this. Treat findings as suggestions to verify.

The latest revision correctly resolves the two issues raised earlier (the NodeKind::Module arm now eyre::bail!s instead of unreachable!, and the redundant validate_module_node_source_fields is gone). Reviewing the current diff, though, I found the following issue:

validate_node_fields_for_kind rejects pattern and output_metadata on Standard nodes, but both are supported node-level fields for standard nodes:

NodeKind::Standard(_) => {
    if !node.output_metadata.is_empty() {
        conflicts.push("output_metadata");
    }
    if node.pattern.is_some() {
        conflicts.push("pattern");
    }
    ...
}

Node::pattern and Node::output_metadata are documented node-level fields (libraries/message/src/descriptor.rs: pattern = "Communication pattern shorthand (e.g. service-server)", output_metadata = per-output metadata keys), and they are actually consumed for standard nodes by check_metadata_annotations in libraries/core/src/descriptor/validate.rs:964, which is called with &node.output_metadata and &node.pattern (the raw node-level fields). So a perfectly valid standard node such as:

- id: server
  path: server.py
  outputs: [response]
  pattern: service-server

now fails with node 'server' has fields that are not supported on its node kind: pattern. Since resolve_aliases_and_set_defaults_in_topology runs on dora run / dora start / the daemon, this makes the documented service/action pattern shorthand (and node-level output_metadata) unusable on standard nodes — a regression for existing dataflows.

Relatedly, the added test invalid_standard_rejects_pattern (and tests/descriptor-validation/cases/invalid-standard-pattern.yml) asserts this rejection as expected behavior, so the test suite locks in the regression rather than catching it. That case should assert the opposite (a standard node with pattern is accepted).

The same rejection on the Operator / Runtime branches looks fine, since for those kinds pattern / output_metadata live under operator.config / operator entries rather than at node level.

Generated by Claude Code

🤖 This is a fully automated review by Claude. No human has vetted these findings — please double-check before acting.

The diff changed substantially since the last automated review — commit 2c95a4b3 swapped the Custom deny-list for a field-merge approach, plus later commits. I re-reviewed the current diff and found one issue that looks like the same class the previous review fixed for Standard nodes, left unfixed for Custom.

Node-level pattern / output_metadata are hard-rejected for Custom nodes, but not for Standard — an inconsistency that turns previously-accepted descriptors into errors.

validate_node_fields_for_kind (libraries/core/src/descriptor/mod.rs, the NodeKind::Custom(_) arm) pushes output_metadata and pattern onto conflicts, so a Custom node that sets either at node level now fails resolution with "has fields that are not supported on its node kind". But:

• These fields exist only on Node / OperatorConfig (libraries/message/src/descriptor.rs:539,546), never on CustomNode / NodeRunConfig — so for a Custom node, node level is the only place they can be expressed (there is no operator.config to move them into).
• check_metadata_annotations (libraries/core/src/descriptor/validate.rs:934-970) iterates every raw node regardless of kind and consumes node.pattern / node.output_metadata at node level — i.e. the framework already treats them as a legitimate node-level annotation for all kinds, Custom included.
• The prior review's Standard-node fix removed exactly this rejection from the Standard arm (now hub-only) and added valid_standard_accepts_pattern / valid_standard_accepts_output_metadata. The Custom arm keeps the rejection, and invalid_custom_rejects_metadata (invalid-custom-metadata.yml) pins it as intended.

Net effect: a service/action node written via custom: (e.g. custom: {source: server.py} + pattern: service-server) resolved fine before this PR and now hard-errors on dora run / dora start, while the same annotation on a Standard node is accepted.

Worth noting for whichever direction you choose: I traced the resolution path and both Standard and Custom nodes resolve into CoreNodeKind::Custom(CustomNode{..}) (mod.rs:172 and :200), and the NodeRunConfig built for the Standard case (mod.rs:183-190) carries neither pattern nor output_metadata — so these are validation-time annotations that don't survive into the resolved node for either kind. That is the same rationale used to keep them legal on Standard, and it applies equally to Custom. Either the Custom arm should also accept pattern / output_metadata, or the asymmetry with Standard deserves a comment explaining why one kind is stricter than the other.

Generated by Claude Code

🤖 Fully automated review by Claude — no human has vetted this. Please verify before acting.

Re-flagging after the newest commit. 94058f8a ("add missing field validation for ROS2 bridge nodes"), pushed after my previous review, extends the node-level pattern / output_metadata rejection to the Ros2Bridge arm of validate_node_fields_for_kind. This is the same class I flagged for Custom nodes last time, and it's still unresolved — the new commit widens it rather than fixing it.

The Custom-node reasoning applies equally to ROS2 bridge nodes:

• Neither kind has an operator.config, so node level is the only place these annotations can be expressed.
• check_metadata_annotations (libraries/core/src/descriptor/validate.rs:964) iterates every raw node regardless of kind and reads node.pattern / node.output_metadata at node level — the exact justification commit 2c95a4b3 used to stop rejecting them on Standard nodes. (For Operator / Runtime, rejecting node-level is fine — there the metadata lives under operator.config, which check_metadata_annotations reads separately.)

So a ROS2 bridge (or Custom) node using the documented pattern shorthand now hard-errors during resolution on dora run / dora start, and the added invalid-ros2-metadata.yml / invalid-custom-metadata.yml fixtures pin that rejection as expected behavior.

Either the Custom and Ros2Bridge arms should accept node-level pattern / output_metadata (consistent with the Standard arm and with check_metadata_annotations), or the asymmetry deserves a comment explaining why these two kinds are stricter. The docs-only commit 257bf2ec introduced no other issues.

Generated by Claude Code

Updated this PR to narrow the scope back to the module-node source-field check only.
Current #2911 now only changes libraries/core/src/descriptor/expand.rs:

• rejects source/kind fields that are mutually exclusive with module
  (path, path_sha256, git, hub, branch, tag, rev, operator,
  operators, custom, ros2)
• keeps env and build out of this rejection, since those have propagation
  semantics
• adds focused unit coverage for both the rejected source fields and the
  accepted env/build case

I also dropped the broader descriptor-field / silent-drop work from this PR.
The root-cause approach for the silent-field-drop issue is now tracked
separately in #3070, which introduces the centralized node field whitelist
classifier for all six node kinds.

🤖 Automated review by Claude — fully automated review; no human has verified these findings.

The PR has been narrowed to a single commit that touches only libraries/core/src/descriptor/expand.rs. This resolves the concerns from the earlier automated reviews and the maintainer's change request — the mod.rs validate_node_fields_for_kind work, the pattern / output_metadata rejections for Custom/ROS2/Standard nodes, the unrelated health_check_timeout doc revert, and the out-of-tree test/doc files are all gone (that broader work moved to #3070).

Re-reviewing the current diff, no issues found:

• validate_module_node_fields runs right after the depth check as the first action in expand_module_node, so it also covers nested module nodes reached via recursion.
• The rejected set (path, path_sha256, git, hub, branch, tag, rev, operators, operator, custom, ros2) matches the source/kind fields on Node; env and build are correctly left allowed since they have propagation semantics.
• Both tests are meaningful: the reject test asserts on both the offending field name and the "mutually exclusive with module" message across all eleven fields, and the allow test verifies env + build pass expansion.

The latest revision looks safe.

Generated by Claude Code